Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore!: remove @waku/message-hash and use @waku/core #1810

Closed
wants to merge 7 commits into from

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Jan 24, 2024

Problem

@waku/message-hash was introduced as a package as an implementation of Deterministic Message Hashing (14/WAKU2-MESSAGE). This package relies on sha256 that comes from @nobles/hashes, and @waku/utils.
Both of these packages are already a dependency in @waku/core so it makes sense to move this packages there.
Plus, the utility of @waku/message-hash, currently, is quite minimal to be setup as a separate package.

Solution

Move message hashing within @waku/core

Notes

cc @fryorcraken

with the removal of `message-hash` as a package, the package-lock was polluted with outdated references
@danisharora099 danisharora099 requested review from a team and weboko January 30, 2024 14:12
Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 31.87 KB (-2.92% 🔽) 638 ms (-2.92% 🔽) 182 ms (-13.12% 🔽) 820 ms
Waku Simple Light Node 301.5 KB (+0.82% 🔺) 6.1 s (+0.82% 🔺) 607 ms (-9.01% 🔽) 6.7 s
ECIES encryption 27.98 KB (-12.55% 🔽) 560 ms (-12.55% 🔽) 100 ms (-57.06% 🔽) 660 ms
Symmetric encryption 27.32 KB (-14.59% 🔽) 547 ms (-14.59% 🔽) 130 ms (-32.13% 🔽) 676 ms
DNS discovery 96.9 KB (-9.21% 🔽) 2 s (-9.21% 🔽) 237 ms (-17.85% 🔽) 2.2 s
Privacy preserving protocols 137.1 KB (+5.89% 🔺) 2.8 s (+5.89% 🔺) 326 ms (+16.29% 🔺) 3.1 s
Light protocols 32.49 KB (+5.25% 🔺) 650 ms (+5.25% 🔺) 65 ms (-78.73% 🔽) 715 ms
History retrieval protocols 30.89 KB (+5.32% 🔺) 618 ms (+5.32% 🔺) 108 ms (-51.09% 🔽) 726 ms
Deterministic Message Hashing 0 B (-100% 🔽) 0 ms (-100% 🔽) 0 ms (-100% 🔽) 0 ms

Copy link
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a lot of other changes slipped into

@fryorcraken
Copy link
Collaborator

Both of these packages are already a dependency in @waku/core so it makes sense to move this packages there.

Not sure how this is justified. The aim is to keep @waku/core lean. @waku/message-hash is not used by core, so why move it in there?

ok, maybe hash and core have similar dependencies... but as long as dependabot ensure that all dependenccies are up to date (ie, core and message-hash use the asme version of the same dependencies) then it should not be a problem.

Until message-hash is actually used in core, this change seems unnecessary.

@danisharora099
Copy link
Collaborator Author

danisharora099 commented Feb 6, 2024

Until message-hash is actually used in core, this change seems unnecessary.

message hashing is indeed being used by the Filter protocol within @waku/core now since we have multiple nodes connected for Filter @fryorcraken

here is it's usage within Filter:

const hashedMessageStr = messageHashStr(

apologies that the PR description did not specify the same!

@danisharora099
Copy link
Collaborator Author

danisharora099 commented Feb 6, 2024

Seems a lot of other changes slipped into

right!
the changes that slipped in are because of update to our lockfile that we had to gen after changing the root package.json because of deprecation to @waku/message-hash
will make a PR to address the protobuf changes separately. thanks!

@danisharora099 danisharora099 marked this pull request as draft February 6, 2024 19:55
@fryorcraken
Copy link
Collaborator

I think it would be a better architect if the multiple filter subscriptions handling is done at a Higher level and Core remains the purest protocol implementation possible+ hard requirements such as peer management.

Fillter redundancy and message deduplication fit at a general SDK level because it's intrinsically opinionated.
Having general SDK logic in core can be problematic in several ways:

  • increases complexity by trying to have a great API while being bound by protocol definition
  • mixed abstraction level. Protocol implementation is one level. Using protocols in a ways that increase reliability, reduces bandwidth usage etc are one level above
  • makes the code less forkable and readable: 99% developers are not expected to ever touch core protocol implementation. But 80/20 rules says 20% of developers will fork or inspires themselves from general SDK to do their own custom usage of the protocols.
  • finally, the protocol implementation should be simple and straightforward for readability and comparisons with the specs. Sebago avoid mixing abstraction levels.

I understand there is a difference with we have done in the past but this is how we solved the problems we previously had.
This way, we don't have to choose between a callback approach or event based.
The protocol implementation needs to be straightforward.

We could even provide several SDK using different approaches: one callback based, one promise based, one event based, etc.

Nwaku and js-waku should some how converge in terms of architects.

core js-waku package is the same level then libwaku nwaku: protocol implementation and mandatory logic such as peer management.

Logic that is here to provide great Dev ex, reliable etc is part of the "compose protocol" milestone scope.
It sits above core/libwaku.

In the case of nwaku. It's logic in node nwaku or written directly in Rust/Python/Golang/NodeJS, using the ffi interface.

In the case of js-waku, it should be a package different from core.

So no, message-hash is currently not used by Waku protocols, you are using it to build a general SDK that composes Waku protocols for better reliability.
Please keep it out of core and put the filter composition in sdk package (or a new one)

@waku-org/js-waku-developers

Cc @waku-org/nwaku-developers

@danisharora099
Copy link
Collaborator Author

danisharora099 commented Mar 5, 2024

tracking the above point here: #1886

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants